Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gen: Refactor command line arguments #2092

Merged
merged 2 commits into from
Nov 8, 2018
Merged

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Nov 5, 2018

Make command line arguments more modular


This change is Reviewable

Copy link
Contributor

@shitz shitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @oncilla)

a discussion (no related file):
This is serviceable, however, I think we should refine this a bit more. Suggestions:

class ArgsBase:
    # Contains all common arguments

class ConfigGenArgs(ArgsBase):
     # Additionally contains all arguments for the ConfigGen

class TopoGenArgs(ArgsBase):
     # Additionally contains all arguments for the TopoGen

class GoGenArgs(ArgsBase):
     # Additionally contains all arguments for the GoGenerator

...

This way all generators have a consistent interface for their constructor and it's easy to extend with new arguments. For example, the TopoGenerator still takes 5 additional arguments even with your changes. Those 5 could go into TopoGenArgs since this approach works also for non-cmdline flag arguments.


@oncilla oncilla force-pushed the gen-refactor branch 2 times, most recently from b85647c to e7bd940 Compare November 7, 2018 12:38
Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on @shitz)

a discussion (no related file):

Previously, shitz wrote…

This is serviceable, however, I think we should refine this a bit more. Suggestions:

class ArgsBase:
    # Contains all common arguments

class ConfigGenArgs(ArgsBase):
     # Additionally contains all arguments for the ConfigGen

class TopoGenArgs(ArgsBase):
     # Additionally contains all arguments for the TopoGen

class GoGenArgs(ArgsBase):
     # Additionally contains all arguments for the GoGenerator

...

This way all generators have a consistent interface for their constructor and it's easy to extend with new arguments. For example, the TopoGenerator still takes 5 additional arguments even with your changes. Those 5 could go into TopoGenArgs since this approach works also for non-cmdline flag arguments.

Done.


Copy link
Contributor

@shitz shitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 11 of 11 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @shitz and @oncilla)


python/topology/common.py, line 41 at r2 (raw file):

    def __init__(self, args, topo_config):
        """
        :param ArgsBase args: Contains the passed command line arguments.

The args are not of type ArgsBase


python/topology/config.py, line 85 at r2 (raw file):

    @classmethod
    def create_parser(cls):

Putting this here makes it less obvious to find. Just create the parser in main.py and pass the args.


python/topology/config.py, line 129 at r2 (raw file):

        """
        self.args = args
        self.topo_config = load_yaml_file(args.topo_config)

self.args for consistency


python/topology/config.py, line 135 at r2 (raw file):

            sys.exit(1)
        self.default_mtu = None
        self._read_defaults(args.network)

here as well


python/topology/config.py, line 307 at r2 (raw file):

            'RegisterTime': 5,
            'PropagateTime': 5,
            'CertChainVersion': 0,

This never seems to be used, but could you change it to 1? Version 0 doesn't exist as such anymore.

Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 11 files reviewed, 6 unresolved discussions (waiting on @shitz)


python/topology/common.py, line 41 at r2 (raw file):

Previously, shitz wrote…

The args are not of type ArgsBase

Done.


python/topology/config.py, line 85 at r2 (raw file):

Previously, shitz wrote…

Putting this here makes it less obvious to find. Just create the parser in main.py and pass the args.

Done.


python/topology/config.py, line 129 at r2 (raw file):

Previously, shitz wrote…

self.args for consistency

Done.


python/topology/config.py, line 135 at r2 (raw file):

Previously, shitz wrote…

here as well

Done.


python/topology/config.py, line 307 at r2 (raw file):

Previously, shitz wrote…

This never seems to be used, but could you change it to 1? Version 0 doesn't exist as such anymore.

Done.

@oncilla oncilla changed the title gen: Refactor command line arguments (PoC) gen: Refactor command line arguments Nov 8, 2018
Copy link
Contributor

@shitz shitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 7 of 7 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


python/topology/common.py, line 41 at r2 (raw file):

Previously, Oncilla wrote…

Done.

The type is argparse.Namespace

Make command line arguments more modular
@oncilla oncilla merged commit 5589019 into scionproto:master Nov 8, 2018
@oncilla oncilla deleted the gen-refactor branch November 8, 2018 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants